Vizzly - Visual Test Results

_{feat/configurable-browser-type · 055b6ee3}

Code Review

This is a solid PR that adds browser configurability with good error handling. Here's my detailed feedback:

✅ Strengths

1. Excellent Error Messages: The browser installation error is clear, actionable, and includes CI-specific guidance. This will significantly improve the developer experience.

2. Good Testing: Config schema validation tests properly cover the new browser type option, including invalid input handling.

3. Sensible Defaults: Defaulting to chromium maintains backward compatibility while enabling flexibility.

4. Chromium-Only Args: Smart decision to only apply chromium-specific args (--no-sandbox, etc.) to chromium, avoiding issues with Firefox/WebKit.

5. Consistent Implementation: Changes are mirrored across both static-site and storybook clients, maintaining consistency.

🔍 Issues & Suggestions

1. Missing Unit Tests for Browser Launching (Medium Priority)

The new error handling and browser type selection logic in launchBrowser() isn't covered by unit tests. The error detection logic (checking for "Executable doesn't exist") could fail silently if Playwright changes their error messages.

Recommendation: Add unit tests for browser.js:

• Test that invalid browser types throw the expected error
• Mock Playwright to test the browser-not-installed error handling
• Verify chromium-specific args are only passed to chromium

Example test location: clients/static-site/tests/browser.test.js

2. Fragile Error Detection (Medium Priority)

if (
  error.message.includes("Executable doesn't exist") ||
  error.message.includes('browserType.launch')
) {

This string matching is brittle. If Playwright changes their error messages in the future, users will get unhelpful errors instead of your nice installation message.

Recommendation:

• Check if Playwright exposes error codes or specific error types
• Consider catching all errors and checking if the browser executable exists on disk as a more reliable detection method
• Add a comment explaining why string matching is necessary (if it is)

3. Code Duplication (Low Priority)

The browser.js file is duplicated between static-site and storybook (only difference is memory limit: 512MB vs 1GB). This creates maintenance burden.

Recommendation: Consider extracting to a shared package (e.g., @vizzly-testing/browser-utils) or at least add a comment explaining why they're separate. For this PR, it's acceptable as-is since it maintains existing patterns.

4. Missing Test for CLI Option (Low Priority)

While parseCliOptions tests exist, there's no integration test verifying that the CLI flag actually flows through to browser launch.

Recommendation: Add a test that verifies:

// clients/static-site/tests/cli-options.test.js or similar
it('--browser flag passes through to launchBrowser', async () => {
  // Test that CLI flag reaches the browser launcher with correct type
});

5. Browser Type Validation Timing (Low Priority)

Browser type is validated in two places:

1. Zod schema (static-site only, at config load time)
2. launchBrowser() function (runtime, both clients)

The storybook client lacks the Zod validation in config-schema. This inconsistency means invalid browser types fail at different times.

Recommendation: Either add browser schema validation to storybook's config-schema or remove it from static-site for consistency. Failing early (at config load) is better UX.

📊 Test Coverage Gap

Based on the diff, I don't see tests for:

• ❌ launchBrowser() with different browser types
• ❌ Browser-not-installed error handling
• ❌ Invalid browser type error message
• ❌ Chromium-specific args isolation

These are critical paths that should have test coverage.

🎯 Security & Performance

No security concerns identified. The browser type is validated before use, preventing injection attacks.

Performance impact is neutral - the changes don't add overhead.

📝 Minor Nitpicks

1. JSDoc: Consider adding @throws annotations to launchBrowser() documenting the error conditions.

2. Consistency: The PR description mentions 178 and 157 tests passing, but doesn't mention adding new tests. The test plan should reference the specific new tests added.

3. Documentation: Consider updating any relevant docs (if they exist) about browser selection for CI caching strategies.

🎬 Recommendation

This PR is functionally sound and ready to merge with the current test coverage, but I'd recommend adding unit tests for browser.js in a follow-up PR to improve robustness. The error detection logic is the main risk area if Playwright changes their error messages.

The excellent error messaging and backward-compatible defaults make this a great UX improvement despite the minor gaps above.

Suggested next steps:

1. Merge as-is if you're comfortable with the current test coverage
2. OR add browser.test.js with mocked Playwright tests before merging
3. File an issue to extract browser.js to shared package (technical debt)